[http-client-csharp-mgmt] code refactoring #50205

haiyuazhang · 2025-05-22T08:49:44Z

Copilot Generated Pull Request Description is accurate.

Copilot

Pull Request Overview

A refactoring PR for the HTTP client management generator, focusing on simplifying method signatures and updating return types.

Introduces new extension methods on InputServiceMethod for handling long-running operations.
Updates several methods to return IReadOnlyList instead of single statements.
Refactors BuildOperationMethod and related methods in ResourceClientProvider to remove the isGeneric parameter and streamline try-catch logic.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
Azure.Generator.Management/src/Utilities/InputMethodExtensions.cs	Adds helper extension methods to determine LRO properties and response types.
Azure.Generator.Management/src/Providers/ResourceCollectionClientProvider.cs	Refactors return types for building method statements and simplifies method call signatures.
Azure.Generator.Management/src/Providers/ResourceClientProvider.cs	Simplifies operation method building by removing the isGeneric parameter and restructuring internal try-catch and pipeline-processing logic.

Comments suppressed due to low confidence (1)

eng/packages/http-client-csharp-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceClientProvider.cs:251

The field '_clientDiagonosticsField' appears to have a typo. Consider renaming it to '_clientDiagnosticsField' for improved clarity and consistency.

UsingDeclare("scope", typeof(DiagnosticScope), _clientDiagonosticsField.Invoke(nameof(ClientDiagnostics.CreateScope), [Literal("${Name}.${signature.Name}")]), out var scopeVariable),

...ient-csharp-mgmt/generator/Azure.Generator.Management/src/Utilities/InputMethodExtensions.cs

...ent-csharp-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceClientProvider.cs

...-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceCollectionClientProvider.cs

ArcturusZhang · 2025-05-26T03:17:38Z

...harp-mgmt/generator/Azure.Generator.Management/src/Utilities/InputServiceMethodExtensions.cs

+            var responseBodyCSharpType = method.GetResponseBodyType();
+            CSharpType rawReturnType = method.GetOperationMethodRawReturnType(resourceClientCSharpType, resourceDataType);


so from below - this responseBodyCSharpType is only used to check if it is null.
the GetOperationMethodRawReturnType did not really use this value, instead, it construct this value again inside the method.

We have quite a few duplicated and confusing logic here.

Also I kind of think that this isAsync is breaking up our core logic - we are having so many ternary operator just doing:

isAsync ? wrap typeof Task : originalType;

I think we could remove this isAsync parameter from this method, and wrap it by isAsync later by another method, such as an extension method to CSharpType WrapAsync(isAsync) which returns you the original type if isAsync is false, or wraps the Task type if isAsync is true.

changes have been made following the review comments.

...-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceCollectionClientProvider.cs

Copilot

Pull Request Overview

This PR refactors the Azure HTTP client management code to improve consistency and modularity in how operation methods are built and executed. Key changes include the introduction of dedicated method providers for resource operations (including Exists and GetIfExists), correction of field naming issues, and improved internal encapsulation of client diagnostics and REST client access.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
InputServiceMethodExtensions.cs	Adds extension methods for identifying long-running operations and extracting response types.
CSharpTypeExtensions.cs	Provides helper methods to wrap CSharpType instances for asynchronous and long-running response handling.
ResourceOperationMethodProvider.cs	Consolidates logic for building operation methods and handling diagnostics, pipelines, and long-running operations.
ResourceCollectionClientProvider.cs	Refactors field naming and delegates operation method construction to specialized providers.
ResourceClientProvider.cs	Refactors operation method creation by delegating to ResourceOperationMethodProvider and updates internal API exposures.
OperationSourceProvider.cs	Removes unnecessary using directives, cleaning up dependencies.
GetIfExistsOperationMethodProvider.cs	Introduces a specialized provider to handle GetIfExists operations with tailored return statement logic.
ExistsOperationMethodProvider.cs	Adds a dedicated provider for Exists operations that converts responses into boolean results.

Copilot

Pull Request Overview

This PR refactors the HTTP management client generator by centralizing operation method creation, introducing reusable extensions, and cleaning up naming inconsistencies.

Added InputServiceMethodExtensions and CSharpTypeExtensions to encapsulate LRO checks and return‐type wrapping.
Extracted operation‐method generation into ResourceOperationMethodProvider and specialized providers for GetIfExists/Exists.
Renamed diagnostic fields for consistency and removed duplicated logic in client/provider classes.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
eng/.../Utilities/InputServiceMethodExtensions.cs	New extensions to detect long‐running operations and derive response body/return types
eng/.../Utilities/CSharpTypeExtensions.cs	New extensions to wrap `CSharpType` in `Task<>`, `Response<>`, or `ArmOperation<>`
eng/.../Providers/ResourceOperationMethodProvider.cs	Centralized method provider handling diagnostics, request/build pipeline, LRO, and returns
eng/.../Providers/ResourceCollectionClientProvider.cs	Fixed typo in diagnostics field name and adjusted visibility of `ResourceClientCSharpType`
eng/.../Providers/ResourceClientProvider.cs	Renamed diagnostics field, delegated operation‐method logic to new provider, exposed internals
eng/.../Providers/OperationSourceProvider.cs	Removed unused `using`
eng/.../Providers/GetIfExistsOperationMethodProvider.cs	Added specialized provider for `GetIfExists` return handling
eng/.../Providers/ExistsOperationMethodProvider.cs	Added specialized provider for `Exists` return handling

...p-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceOperationMethodProvider.cs

...harp-mgmt/generator/Azure.Generator.Management/src/Utilities/InputServiceMethodExtensions.cs

Copilot

Pull Request Overview

This PR refactors the HTTP client management generator code to improve maintainability and consistency across various provider and extension classes. Key changes include the addition of helper extension methods for CSharp types and input service methods, as well as significant restructuring of operation method providers for update, exists, get-if-exists, and get-all operations.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
Utilities/InputServiceMethodExtensions.cs	Added extension methods for determining operation characteristics and return types.
Utilities/CSharpTypeExtensions.cs	Provided helper methods to wrap and unwrap CSharpType instances for async/long-running operations.
Providers/UpdateOperationMethodProvider.cs	Introduced a dedicated provider for update operations.
Providers/ResourceOperationMethodProvider.cs	Refactored the base logic for operation methods, including diagnostic scoping and request processing.
Providers/ResourceCollectionClientProvider.cs	Updated field names and adjusted contextual parameter handling for collection clients.
Providers/ResourceClientProvider.cs	Renamed diagnostic field and streamlined operation method creation for resource clients.
Providers/OperationSourceProvider.cs	Removed unused using directive.
Providers/GetIfExistsOperationMethodProvider.cs	Created a provider for get-if-exists operations with improved signature handling.
Providers/GetAllResourceOperationMethodProvider.cs	Added a provider for get-all operations with a placeholder for paging implementation.
Providers/ExistsOperationMethodProvider.cs	Developed a dedicated provider for checking resource existence.

.../generator/Azure.Generator.Management/src/Providers/GetAllResourceOperationMethodProvider.cs

...p-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceOperationMethodProvider.cs

.../generator/Azure.Generator.Management/src/Providers/GetAllResourceOperationMethodProvider.cs

...p-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceOperationMethodProvider.cs

…Provider

ArcturusZhang · 2025-05-29T03:51:14Z

...lient-csharp-mgmt/generator/Azure.Generator.Management/src/Utilities/CSharpTypeExtensions.cs

+
+        public static CSharpType UnWrapAsync(this CSharpType type)
+        {
+            if (type.IsGenericType && type.Name == "Task")


I think for types, comparing with names would always be problematic.
We could do this instead:

type.IsFramework && type.FrameworkType.GetGenericTypeDefinition() == typeof(Task<>)

to 100% sure identify the Task type, instead of catching some model with the name of Task for example

ArcturusZhang · 2025-05-29T03:51:48Z

...lient-csharp-mgmt/generator/Azure.Generator.Management/src/Utilities/CSharpTypeExtensions.cs

+                if (type.Arguments.Count != 1)
+                {
+                    throw new InvalidOperationException($"Task type must have exactly one type argument, but found {type.Arguments.Count}.");
+                }


I think if we do the above changes, we could be confident and remove this throw.

Copilot AI review requested due to automatic review settings May 22, 2025 08:49

haiyuazhang requested review from JoshLove-msft, m-nash, jorgerangel-msft, live1206 and ArcturusZhang as code owners May 22, 2025 08:49

github-actions bot added CodeGen Issues that relate to code generation Mgmt This issue is related to a management-plane library. labels May 22, 2025

Copilot AI reviewed May 22, 2025

View reviewed changes

haiyuazhang marked this pull request as draft May 22, 2025 08:50

haiyuazhang marked this pull request as ready for review May 22, 2025 09:13

haiyuazhang force-pushed the haiyzhan/mgmt-generator/code_refactory branch 2 times, most recently from 354e449 to aab13f8 Compare May 22, 2025 13:54

haiyuazhang marked this pull request as draft May 22, 2025 16:32

live1206 reviewed May 23, 2025

View reviewed changes

haiyuazhang force-pushed the haiyzhan/mgmt-generator/code_refactory branch from 4fc9a68 to d293824 Compare May 23, 2025 05:01

haiyuazhang added 9 commits May 23, 2025 14:39

refine method BuildOperationMethodTryStatement.

21d9dec

address remaining unresolved review comments from ealier PR.

e0dbe19

remove useless comments.

8fff469

fix typo.

94935f0

new tspCodeModel.json.

05048c1

regen tspCodeModel.json.

1a8bc7c

fix rebase issue.

7169709

address review comments.

d5fb687

fix.

7e0e865

haiyuazhang force-pushed the haiyzhan/mgmt-generator/code_refactory branch from 9702022 to 7e0e865 Compare May 23, 2025 09:18

haiyuazhang marked this pull request as ready for review May 23, 2025 09:34

ArcturusZhang reviewed May 26, 2025

View reviewed changes

...-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceCollectionClientProvider.cs Outdated Show resolved Hide resolved

address code review comments.

6fb2920

haiyuazhang added 2 commits May 27, 2025 23:35

create new class ResourceOperationMethodProvider.

c41fe55

add two extension methods to CSharpType.

b1e73e3

haiyuazhang requested a review from Copilot May 28, 2025 01:44

Copilot AI reviewed May 28, 2025

View reviewed changes

haiyuazhang requested review from live1206, Copilot and ArcturusZhang May 28, 2025 01:44

Copilot AI reviewed May 28, 2025

View reviewed changes

...p-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceOperationMethodProvider.cs Outdated Show resolved Hide resolved

...p-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceOperationMethodProvider.cs Outdated Show resolved Hide resolved

address new review comments from copilot.

e75c46a

live1206 reviewed May 28, 2025

View reviewed changes

haiyuazhang added 5 commits May 28, 2025 17:43

further refine.

9c227c7

more refine.

60c5cba

Apply WrapAsync everywhere possible.

bc35add

Replace SkipMethodParameter with ImplicitParameterNames property

a357be8

introduce GetAllResourceOperationMethodProvider.

299398b

haiyuazhang requested a review from Copilot May 28, 2025 17:17

Copilot AI reviewed May 28, 2025

View reviewed changes

.../generator/Azure.Generator.Management/src/Providers/GetAllResourceOperationMethodProvider.cs Outdated Show resolved Hide resolved

live1206 reviewed May 29, 2025

View reviewed changes

haiyuazhang added 3 commits May 29, 2025 10:56

address new review comments.

544d388

rename GetAllResourceOperationMethodProvider to GetAllOperationMethod…

b7864a3

…Provider

small refine.

7f4f721

ArcturusZhang reviewed May 29, 2025

View reviewed changes

		var responseBodyCSharpType = method.GetResponseBodyType();
		CSharpType rawReturnType = method.GetOperationMethodRawReturnType(resourceClientCSharpType, resourceDataType);

[http-client-csharp-mgmt] code refactoring #50205

Are you sure you want to change the base?

[http-client-csharp-mgmt] code refactoring #50205

Conversation

haiyuazhang commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ArcturusZhang May 26, 2025

Choose a reason for hiding this comment

Uh oh!

ArcturusZhang May 26, 2025

Choose a reason for hiding this comment

Uh oh!

haiyuazhang May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ArcturusZhang May 29, 2025

Choose a reason for hiding this comment

Uh oh!

ArcturusZhang May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

haiyuazhang commented May 22, 2025 •

edited

Loading